chore(metrics): use geth metrics package and delete local metrics #1422

qdm12 · 2025-01-13T13:06:05Z

Why this should be merged

Contribution to the libevm effort.
Note there are the same changes for both coreth and subnet-evm, but they should not be part of libevm since these are extra features specific to subnet-evm/coreth, even if they're the same for both.

How this works

Comparing the following:

subnet-evm master vs coreth master: subnet-evm is missing the extra file https://github.com/ava-labs/coreth/blob/b6b4dfbc4bfc7f17e4cfe5f3c0fb44944176c884/metrics/init_test.go (which is present in geth as well)
subnet-evm master vs geth v1.13.14: subnet-evm has many deletions, very few minor unneeded changes and the new non-conflicting file metrics/prometheus/prometheus.go

Therefore:

file kept and refactored metrics/prometheus/prometheus.go with the Gatherer implementation we use
file kept and refactored metrics/prometheus/prometheus_test.go
new file metrics/prometheus/interfaces.go added for refactoring
geth global variable metrics.Enabled is set to true in plugin/evm.VM.initializeMetrics

Note for the two files refactored (with fixes as well), this was done in some intermediary step in ava-labs/libevm#103, so I decided to bring this over here so it doesn't get trashed.

How this was tested

CI passing

Need to be documented?

No

Need to update RELEASES.md?

Not really, since it should not change anything 🙏

plugin/evm/vm.go

warp/handlers/signature_request_test.go

metrics/prometheus/prometheus.go

metrics/prometheus/prometheus_test.go

metrics/prometheus/prometheus.go

go.mod

warp/handlers/signature_request_test.go

metrics/prometheus/prometheus.go

maru-ava

Looks good! The subnet dashboard is now populated with metrics for the load test e2e job:

A random PR doesn't show any metrics on the subnets dashboard for the same job:

qdm12 · 2025-01-28T12:11:33Z

A random PR doesn't show any metrics on the subnets dashboard for the same job:

Ah, let me check if I can revert it to no-metric, and open another PR (based off this one I suppose) to enable metrics.
Since this PR is not meant to change anything and should be pure maintenance / code movement.

qdm12 · 2025-02-10T07:49:50Z

Working as expected, this is good to go (same as ava-labs/coreth#745), just need reviews 😉

darioush

some optional suggestions

scripts/geth-allowed-packages.txt

plugin/evm/testutils/testutils_test.go

plugin/evm/testutils/metrics.go

plugin/evm/testutils/testutils_test.go

plugin/evm/vm.go

scripts/geth-allowed-packages.txt

Signed-off-by: Quentin McGaw <[email protected]>

- add exported comments - rename `g` to `gatherer` in test - Format copyright as it should be ™️ - Remove TODO

…in/evm

Signed-off-by: Quentin McGaw <[email protected]>

- remove non linkable prometheus.Registry

Co-authored-by: Arran Schlosberg <[email protected]> Signed-off-by: Quentin McGaw <[email protected]>

qdm12 · 2025-03-21T10:25:46Z

Force push rebasing the branch on qdm12/libevm/master-merge-1 (libevm up to date with master), history is kept more or less, but everything will have to be re-reviewed.

this PR got merged and I have no idea how. I may have pressed keyboard shortcuts merging it, but I really have no idea, I re-opened another draft PR #1508 + hard reset my branch qdm12/libevm/master-merge-1 to discard that out-of-nowhere-merge.

qdm12 mentioned this pull request Jan 13, 2025

chore(metrics/prometheus): add files needed for coreth and subnet-evm ava-labs/libevm#103

Closed

qdm12 force-pushed the qdm12/libevm/metrics branch 2 times, most recently from 34e9afd to 8c8514c Compare January 13, 2025 14:57

qdm12 commented Jan 13, 2025

View reviewed changes

qdm12 marked this pull request as ready for review January 13, 2025 16:08

qdm12 requested review from a team, ceyonur and darioush as code owners January 13, 2025 16:08

qdm12 commented Jan 13, 2025

View reviewed changes

metrics/prometheus/prometheus.go Outdated Show resolved Hide resolved

metrics/prometheus/prometheus_test.go Outdated Show resolved Hide resolved

qdm12 commented Jan 13, 2025

View reviewed changes

metrics/prometheus/prometheus.go Outdated Show resolved Hide resolved

qdm12 mentioned this pull request Jan 13, 2025

chore(metrics): use geth metrics package and delete local metrics ava-labs/coreth#745

Merged

darioush reviewed Jan 13, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

warp/handlers/signature_request_test.go Outdated Show resolved Hide resolved

metrics/prometheus/prometheus.go Outdated Show resolved Hide resolved

qdm12 force-pushed the qdm12/libevm/metrics branch from 7c08815 to 15866c8 Compare January 15, 2025 11:15

qdm12 requested a review from darioush January 15, 2025 11:40

darioush previously approved these changes Jan 23, 2025

View reviewed changes

qdm12 changed the title ~~chore(metrics): use libevm metrics package and delete local metrics~~ chore(metrics): use geth metrics package and delete local metrics Jan 27, 2025

qdm12 force-pushed the qdm12/libevm/metrics branch 2 times, most recently from 769d5fc to b008bf7 Compare January 27, 2025 13:33

maru-ava previously approved these changes Jan 27, 2025

View reviewed changes

qdm12 dismissed stale reviews from maru-ava and darioush via a4eaf83 February 7, 2025 14:34

qdm12 enabled auto-merge (squash) February 10, 2025 07:50

qdm12 requested review from darioush and maru-ava February 10, 2025 07:50

maru-ava previously approved these changes Feb 10, 2025

View reviewed changes

darioush previously approved these changes Feb 11, 2025

View reviewed changes

scripts/geth-allowed-packages.txt Outdated Show resolved Hide resolved

plugin/evm/testutils/testutils_test.go Outdated Show resolved Hide resolved

ceyonur reviewed Feb 11, 2025

View reviewed changes

qdm12 dismissed darioush’s stale review via e389c7f February 17, 2025 13:02

qdm12 and others added 24 commits March 21, 2025 10:56

Change copyright notice from 2025 to 2021-2025

7b3611a

Signed-off-by: Quentin McGaw <[email protected]>

Enable libevm metrics in initializeMetrics

51d7255

testutils.WithMetrics function in plugin/evm

3a95319

metrics/prometheus: add test cases for full coverage

6e58856

metrics/prometheus: add metric name to error wrappings

4949b7d

Minor changes

d03d034

- add exported comments - rename `g` to `gatherer` in test - Format copyright as it should be ™️ - Remove TODO

Enforce testutils package is only imported by test files outside plug…

bd8a136

…in/evm

Rework regex matching for lint_allowed_geth_imports.sh

0183ff5

Rework TestMustNotImport to check for imports in non test code

29e39f0

Add comment on metrics.Enabled = true in initializeMetrics

3672a2e

Clarify testutils package comment

4e9cf9e

Signed-off-by: Quentin McGaw <[email protected]>

Skip nil metric types

a7a8c24

TestGatherer_Gather tests nil metrics

c6b2e22

Remove restrictions on plugin/evm/testutils package imports

8207254

Move plugin/evm/testutils to internal/testutils

c89a5a5

Add mutex for WithMetrics call

710f0ca

Improve Registry comment

abb2a70

- remove non linkable prometheus.Registry

Improve readability of Gather() body for error handling

81c7f8a

Use testutils.WithMetrics in TestGatherer_Gather

4004860

Use sentinel error for metric type not supported

86e1086

Add missing copyright notice to testutils/metrics.go

0c9f6df

registerNilMetrics function

2166bea

Add context to require.NoError in test

bcc3919

Co-authored-by: Arran Schlosberg <[email protected]> Signed-off-by: Quentin McGaw <[email protected]>

Replace Registry comment with interface assertion

2ce6cfa

qdm12 force-pushed the qdm12/libevm/metrics branch from 92a9a1e to 2ce6cfa Compare March 21, 2025 10:14

qdm12 changed the base branch from master to qdm12/libevm/master-merge-1 March 21, 2025 10:14

qdm12 merged commit 216c6a4 into qdm12/libevm/master-merge-1 Mar 21, 2025
3 of 13 checks passed

qdm12 deleted the qdm12/libevm/metrics branch March 21, 2025 10:17

qdm12 mentioned this pull request Mar 21, 2025

chore(metrics): use libevm metrics package #1508

Merged

chore(metrics): use geth metrics package and delete local metrics #1422

chore(metrics): use geth metrics package and delete local metrics #1422

Uh oh!

Conversation

qdm12 commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented?

Need to update RELEASES.md?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maru-ava left a comment

Choose a reason for hiding this comment

Uh oh!

qdm12 commented Jan 28, 2025

Uh oh!

qdm12 commented Feb 10, 2025

Uh oh!

darioush left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qdm12 commented Mar 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qdm12 commented Jan 13, 2025 •

edited

Loading